Further asyncify the codebase #890
Conversation
jprendes
force-pushed
the
mostly-async
branch
7 times, most recently
from
1b01571 to
36a7cf5
Compare
|
There's an odd CI failure here |
There was a problem hiding this comment.
PR Overview
This pull request introduces async/await throughout the codebase while keeping a synchronous interface where needed by using the ambient Tokio runtime. Key changes include refactoring existing synchronous APIs to async versions (with adjustments in instance creation, start, kill, delete, and wait methods), updating tests to run under Tokio, and switching dependencies and types (e.g. using tokio’s RwLock and replacing ExitSignal with WaitableCell).
Reviewed Changes
| File | Description |
|---|---|
| typos.toml | Added new rules to ignore false‐positive typos. |
| crates/containerd-shim-wasm/src/testing.rs | Converted instance operations to use async (with .block_on()) in sync contexts. |
| crates/containerd-shim-wasm/src/sandbox/shim/local.rs | Changed several methods from blocking to async, updating lock handling and usage of get_instance, task_start, task_kill, etc. |
| Cargo.toml | Updated Tokio features and added new dependencies (trait-variant, tokio-async-drop). |
| crates/containerd-shim-wasm/src/sandbox/cli.rs | Adjusted CLI to run prehook code within an async block and updated exit handling. |
| Various tests | Updated tests to use Tokio’s async runtime and channels, including replacing std::sync channels with Tokio channels. |
| Instance APIs (instance_data.rs, instance.rs, etc.) | Updated trait definitions and implementations to be fully async, replacing OnceLock with OnceCell and adjusting sync wrappers. |
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
crates/containerd-shim-wasm/src/sandbox/shim/local.rs:124
- Review the use of .block_on() in synchronous trait implementations that rely on this async function, as it may risk deadlocking if the ambient Tokio runtime is not properly isolated. Consider propagating async context or creating dedicated sync wrappers to reduce the risk.
pub(super) async fn get_instance(&self, id: &str) -> Result<Arc<InstanceData<T>>> {
crates/containerd-shim-wasm/src/sandbox/shim/instance_data.rs:40
- Ensure that all async calls (like instance start) are properly awaited and that the ambient runtime is initialized to avoid unintentional blocking; verify that converting these methods to async does not introduce execution order issues in production.
let res = self.instance.start().await;
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
|
rebased |
… c8d leases Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
| // It will stop executing when dropped. | ||
| // We need to keep this future's lifetime tied to this | ||
| // method's lifetime. | ||
| // This means we shouldn't tokio::spawn it, but ruther |
There was a problem hiding this comment.
| // This means we shouldn't tokio::spawn it, but ruther | |
| // This means we shouldn't tokio::spawn it, but rather |
TL.DR.: This PR sprinkles async and await throughout the codebase.
This PR further introduces async to the codebase without the leap of switching to async rust-extensions.
The reason not to switch to async rust-extensions yet is that it doesn't work on windows (I.(i.e., it doesn't build on Windows, so our CI fails).
There are a few options here for the future:
This PR uses our "ambient" tokio runtime to run our async codebase inside of the sync methos that rust-extensions expects.
This PR doesn't convert our testing framework to async either. This is to reduce the amount of changes.